Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ndpi: ndpi as a plugin - v2 #12120

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jasonish
Copy link
Member

Just a rebase and non-draft PR now that the support work has been merged into master.

cc: @lucaderi @cardigliano

Ticket: https://redmine.openinfosecfoundation.org/issues/7231

@jasonish jasonish requested review from jufajardini and a team as code owners November 14, 2024 08:48
@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 23321

@victorjulien
Copy link
Member

How would testing this in SV work? Is there a way to know in a test that ndpi is available?

@jasonish
Copy link
Member Author

How would testing this in SV work? Is there a way to know in a test that ndpi is available?

I think S-V will need a small update to pick out enabled built-in plugins. I'm not sure if we have to add a "HAVE_XXX" for something that is not statically enabled.

We already have some support for this in S-V, but it just doesn't pickup stuff under the "Plugin" section of --build-info, but trivial to add.

The other question is if fields that are added to the logs by plugins should go into the schema as well?

@victorjulien
Copy link
Member

How would testing this in SV work? Is there a way to know in a test that ndpi is available?

I think S-V will need a small update to pick out enabled built-in plugins. I'm not sure if we have to add a "HAVE_XXX" for something that is not statically enabled.

We already have some support for this in S-V, but it just doesn't pickup stuff under the "Plugin" section of --build-info, but trivial to add.

The other question is if fields that are added to the logs by plugins should go into the schema as well?

This will require us to load the plugins and check the config before build info, right?

Wrt the schema, would it make sense to have a per plugin additional schema that is somehow used for those records?

@jasonish
Copy link
Member Author

This will require us to load the plugins and check the config before build info, right?

This PR has the following in --build-info:

Plugins:
  nDPI:                                    yes

S-V could also test that the plugin.so file exists as well.

@jasonish
Copy link
Member Author

jasonish commented Nov 14, 2024

Wrt the schema, would it make sense to have a per plugin additional schema that is somehow used for those records?

Will require research to see if JSON schema can be extended from another file, and if the schema validation libs we use would support doing that as well.

@victorjulien
Copy link
Member

This will require us to load the plugins and check the config before build info, right?

This PR has the following in --build-info:

Plugins:
  nDPI:                                    yes

S-V could also test that the plugin.so file exists as well.

So this tells us it is built, but not necessary loaded, right? So then the SV test would either need to load the plugin, or it would need to be able to check if it is loaded?

@jasonish
Copy link
Member Author

jasonish commented Nov 15, 2024

This will require us to load the plugins and check the config before build info, right?

This PR has the following in --build-info:

Plugins:
  nDPI:                                    yes

S-V could also test that the plugin.so file exists as well.

So this tells us it is built, but not necessary loaded, right? So then the SV test would either need to load the plugin, or it would need to be able to check if it is loaded?

If this is yes, the plugin path in suricata.yaml is the install path, so still not good enough to check if loaded.

So probably have to do something like:

requires:
  files:
    - plugins/ndpi/ndpi.so

args:
  - --set plugins.0=./plugins/ndpi/ndpi.so

@victorjulien
Copy link
Member

Do we need the plugin register a feature for using the requires keyword? Do we need to also expose the ndpi version to that keyword?

@jasonish
Copy link
Member Author

jasonish commented Nov 15, 2024

Do we need the plugin register a feature for using the requires keyword? Do we need to also expose the ndpi version to that keyword?

Probably should register a feature here, will do. Should just work from a plugin but I don't think I've tried yet.

Not quite sure how to require a specific version of a "feature" yet. We didn't put that into our use case mapping. Will require some thought.

@jasonish
Copy link
Member Author

Not quite sure how to require a specific version of a "feature" yet. We didn't put that into our use case mapping. Will require some thought.

requires: ndpi, ndpi.version >= 1.2.3

ndpi just being an example here, but make this last .version component generic over all plugins and add version registration to the plugin. It would be the version of the plugin, not the underlying library though.

The other option is to register more features, like:

  • ndpi.risk_keyword
  • ndpi.foobar_keyword

Just thinking out loud.

@jasonish
Copy link
Member Author

jasonish commented Nov 20, 2024

Or maybe something like: #12138

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants